Summary

The diagnosis is right, the two-line fix is right, and the tests are the kind this repo
should have more of. I reproduced everything independently: extracted ShortenPath into a
standalone harness in both its origin/main and post-PR forms and ran all 12 pinned cases.
Post-fix 12/12 pass; pre-fix exactly 2 fail (a/.. → a/.., ../.. → "") and
ShortenPath(L"..", true) dies under ASan with SEGV on unknown address ... caused by a WRITE memory access inside basic_string(basic_string&&) — the pop_back() on an empty
string. That matches the PR's "2 failures plus a SEGFAULT" claim precisely. The eight-caller
inventory is exact, and every one of them really is a containment check.

One substantive item before merge. The impact section measures only the starts_with(root)
form of the guard and reports "exactly one verdict flips". I swept the same change through
all the caller predicates and the three "../"-prefix guards flip 16 inputs in the
deny→allow direction, because paths of the shape ../X/.. now resolve to exactly ".." —
two characters, so a check requiring size() > 3 and a "../" prefix no longer fires. I
chased each one down to the actual file operation and none of them escape (details below), so
this is not a blocker, but the PR's central justification is a security-impact analysis and
right now it is incomplete in the one direction a reader cares about.

CI has now run — the gated workflows were released on 2026-08-27 — and it backs the PR up.
The Linux leg is green and path_test genuinely compiled, linked and executed under CTest:
Linking CXX executable path_test/path_test, then 9/9 Test #9: path_test ... Passed, with
the run ending 100% tests passed, 0 tests failed out of 8 (the shortfall is ooxml_test
being Not Run (Disabled), pre-existing). WASM is green too. The Windows leg is red, but for
a reason that has nothing to do with this PR — see below.

Verification

Issues & Suggestions

⚠️ Major

• The impact analysis measured one of the two guard styles, and the unmeasured one moves
  the other way. The PR says "Measured over Combine(root, rel), exactly one verdict
  flips". That covers the five starts_with(root) callers. It does not cover the three that
  test for a "../" prefix — ZipUtilsCP.cpp:296, CEpubFile.cpp:167, CImage.cpp:85 (and
  the wsCorePath.empty() fallback in both CanUseThisPath implementations).

  Sweeping every path over the segment alphabet {a, b, ., ..} up to length 4, with and
  without a leading/trailing separator, through those predicates gives 28 allow→deny and
  16 deny→allow. Every one of the 16 has the same shape: ../X/.. (plus .-noise
  variants like .././a/.., ../a/./..) previously resolved to ../X/.. and was rejected;
  it now correctly resolves to ".." — which is 2 characters, so size() > 3 fails and
  substr(0, 3) == "../" never matches. ".." is an escape, and the guards no longer see
  it.

  I chased all of them to the actual file operation and none escape, so I am not calling
  this blocking:

  • ZipUtilsCP.cpp: output is built from the raw filenameW (ZipUtilsCP.cpp:267), so
    the guard is the only thing in the way — but a resolved form of ".." means the write
    target ends in /.., and CreateFileW on a directory fails.
  • CEpubFile.cpp:173: sFile == ".." yields <tmp>/<content>/.., handed to
    ConvertHTML2OOXML as a file to read. A directory; the read fails.
  • CImage.cpp:88: <workdir>/.. then CFileBinary::Exists → false.

  What I would like before merge is small: pin the behaviour with one more test so nobody has
  to re-derive this, and correct the claim.

  // ../X/.. resolves to a bare "..", which is still outside the base. The "../"-prefix
  // guards in ZipUtilsCP, CEpubFile and CImage require >= 3 characters and so do not
  // catch it; only the starts_with(root) callers reject it. Pinned so the gap is visible.
  TEST(ShortenPath, collapses_a_parent_chain_to_a_bare_parent)
  {
      EXPECT_EQ(L"..", NSSystemPath::ShortenPath(L"../b/.."));
  }

  and reword the impact paragraph along the lines of: "Measured over Combine(root, rel)
  for the five starts_with(root) callers, exactly one verdict flips. The three
  "../"-prefix callers tighten in 28 cases and loosen in 16, all of the form ../X/.. →
  "..", which their size() >= 3 test misses; in each case the resulting path names a
  directory and the subsequent file operation fails, so no traversal materialises. Tightening
  those guards to also reject ".." and "" is left to a follow-up."

  The follow-up itself is genuinely out of scope here — worth noting that #117 already adds
  if (wsNormalizedFilename.empty()) return UNZ_INTERNALERROR; to ZipUtilsCP, so half of
  it lands there if that PR merges. The two PRs don't conflict (different files) but #137
  changes the semantics #117 depends on, so whichever merges second wants a re-read.

• The red Windows check is not yours — but it should be named, not merged past silently.
  Build (Windows x64) fails in CMake Configure, in depot_tools' V8 sync
  (git_cache.py:217 GetCachePath() → FileNotFoundError: [WinError 2]), before Path.cpp
  is compiled or DesktopEditor/common/test is configured. Nothing in this diff can reach
  it, and it reproduces on every recent branch. #132 (fix(v8): ensure a git shim exists in depot_tools on Windows) is the fix, and the only PR with a green Windows leg since
  2026-08-15; it was amended on 2026-08-28 and is now DCO-green with its builds re-running.
  That is a repo-health item rather than a #137 item, but until it lands this PR merges
  without Windows validation.

ℹ️ Minor

• keeps_an_absolute_path is platform-dependent. ShortenPath only re-attaches the
  leading / under #if !defined(_WIN32) && !defined(_WIN64) (Path.cpp:249), so
  EXPECT_EQ(L"/tmp/evil.txt", ...) at path.cpp:52 returns tmp/evil.txt on Windows.
  Harmless today — build-windows.yml never sets -DEO_BUILD_TESTS=ON, so the suite isn't
  built there — but it will bite whoever turns Windows tests on. Either guard the expectation
  with the same #if, or leave a one-line comment saying the case is POSIX-only.

• No license header on the two new files. Worth stating plainly what is and isn't a
  problem here. The good part: path.cpp does not carry a copy-pasted
  (c) Copyright Ascensio System SIA 2010-2019 block, which is the mistake I keep seeing on
  new files in these forks — it would misattribute fork-authored work and drag in the AGPL
  §7(a) and CC-BY-SA clauses that are specifically ONLYOFFICE's. Avoiding that was right.

  The gap is that there is no header at all. In this repo that is consistent with every
  precedent: core has zero SPDX-License-Identifier lines anywhere, and all three
  fork-authored test CMakeLists (OOXML/test, Common/cfcpp/test, OdfFile/Test/test_odf)
  and OfficeUtils/tests/main.cpp are equally bare. So this is a convention the repo has
  never set, not something this PR broke — which is exactly why it's worth setting once,
  across #136 and #137 together, rather than PR by PR. The form the org already uses in its
  own .github repo:

  /*
   * SPDX-FileCopyrightText: 2026 Euro-Office contributors
   * SPDX-License-Identifier: AGPL-3.0-or-later
   */
  

  (# for the CMakeLists.) Files that genuinely derive from Ascensio code keep the upstream
  notice and gain the Euro-Office line beneath it; only from-scratch files like these two get
  the Euro-Office header alone. Happy for this to be a separate PR if you'd rather not
  reopen #136/#137 for it.

• TESTING.md isn't updated. That file is the register of CTest suites — it opens with
  "13 are real GoogleTest suites" and its Migration status → Done list enumerates every one.
  path_test makes 14 and is the first suite that isn't a qmake migration at all, so it
  doesn't fit the existing headings. A short "New suites (not from qmake)" bullet would keep
  the doc honest. #136 has the same gap; one entry covering both would do.

• No linked issue. "Own find, not from a report" is fine, and the org's issue-first rule
  is aimed at features rather than drive-by bug fixes. But two things in this PR deserve to
  outlive it: ShortenPath(L"/") returning "" instead of /, and the size() >= 3 gap in
  the three prefix guards. An issue for each would stop them being rediscovered from scratch.

💡 Suggestions

• Concrete resolution for the #136 collision. Rather than merging the two CMakeLists by
  hand into something ad hoc, whichever lands second can make the directory hold both
  targets — that scales as more DesktopEditor/common tests arrive:

  project(common_test)
  ...
  add_core_gtest(NAME path_test      SOURCES path.cpp      LIBS kernel GTEST_MAIN)
  add_core_gtest(NAME directory_test SOURCES directory.cpp LIBS kernel GTEST_MAIN)

  with a single root line add_subdirectory( "${CORE_ROOT_DIR}/DesktopEditor/common/test" common_test ).
  That also wants #136's main.cpp renamed to directory.cpp, matching this PR's choice of
  path.cpp — main.cpp is a poor name for a file with no main() (GTEST_MAIN generates it).

• The bRemoveExternalPath guard could arguably return L"/" for an absolute input instead
  of "", since /.. is /. I'd leave it as is — matching the existing arStack.empty()
  return keeps one convention instead of two, and "" fails safe under starts_with(root).
  Noting it only so the choice reads as deliberate.

Code quality & conventions

• Commit: fix(common): resolve a trailing ".." in NSSystemPath::ShortenPath —
  Conventional Commits, correct type and scope. The body is genuinely good: it states the
  defect, the mechanism, the caller impact, the verification method, and the one thing left
  unfixed. That is the standard I'd like other PRs here to hit.
• DCO: Signed-off-by: Krzysztof Cieślik present, check green.
• AI disclosure: compliant — PR body section plus Assisted-by: Claude Code:claude-opus-5
  and Co-Authored-By: trailers.
• Tests: present, and they are the right tests — pure-function cases with no fixtures, 3
  reproducing the defects and 9 pinning behaviour that must not move. Verified both ways by
  the author and independently by me. This is what a behavioural fix in this repo should look
  like.
• Scope: focused. One function, two changes, plus the test scaffolding those changes need.
• Comments: the block at Path.cpp:240-242 and the header comment in path.cpp both
  explain why rather than restating the code. Kept.

Verdict

Comment — the fix is correct, minimal, and properly tested; I verified the defect, the
crash, the caller inventory and all 12 expectations independently, and they all hold, and CI
now confirms path_test builds and passes under CTest. One ask before merge: extend the
impact analysis to the "../"-prefix guards — it currently reports a net tightening while 16
inputs loosen, none of them exploitable, which is one extra test and a reworded paragraph.
The red Windows leg is the pre-existing depot_tools breakage (#132), not this PR. The
license-header and TESTING.md points are conventions to settle across #136 and #137
together, not reasons to hold this PR.

Assisted-by: ClaudeCode:claude-opus-5